-
Notifications
You must be signed in to change notification settings - Fork 7.1k
[Serve] Add label_selector and bundle_label_selector to Serve API
#57694
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
[Serve] Add label_selector and bundle_label_selector to Serve API
#57694
Conversation
Signed-off-by: Ryan O'Leary <[email protected]>
|
cc: @MengjinYan @eicherseiji I think this change can help enable TPU use-cases with Ray LLM, since it'll allow users to target the desired slice/topology based on labels like these: |
MengjinYan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current change looks good.
At the same time, I have a general question, probably also to @eicherseiji as well: I'm not familiar with the serve codebase. But from the look at the code, the change in the PR seems not cover the whole code path from the replica config to actually creating the placement group where we need to apply the bundle label selector (e.g. CreatePlacementGroupRequest, DeploymentTargetState, DeploymentVersion, etc.).
Wondering if we should include the change to the rest of the code path in this PR as well?
|
Hi @ryanaoleary! Seems like it may be useful, but could you go into more detail about the problem this solved for you? I.e. is it typical to have multiple TPU node pools/heterogeneous compute in a TPU cluster? |
Signed-off-by: Ryan O'Leary <[email protected]>
The use-case would be for TPU multi-slice, where it's important that the Ray nodes reserved with some group of resources (i.e. a bundle of TPUs) are a part of the same actual TPU slice so that they can benefit from the high speed ICI interconnects. There are GKE webhooks that set some TPU information as env vars when the Pods are created, including topology and a unique identifier for the slice which we set as Ray node labels. They also inject For a RayCluster with multiple TPU slices (of the same topology or different), we currently only schedule using |
|
@ryanaoleary I see, thanks! Is there a reason we can't extend/re-use the |
In addition to the TPU support, in general, we want to have label support in all library APIs so that user can do scheduling based on node labels as well. |
I think just configuring |
|
This pull request has been automatically marked as stale because it has not had You can always ask for help on our discussion forum or Ray's public slack channel. If you'd like to keep this open, just leave any comment, and the stale label will be removed. |
|
@ryanaoleary I will review this PR next week |
|
thanks for working on this! Really looking forward to using label selectors in Serve. |
Signed-off-by: ryanaoleary <[email protected]>
Signed-off-by: ryanaoleary <[email protected]>
| # Actor: Use Actor label selector | ||
| if "label_selector" in scheduling_request.actor_options: | ||
| primary_labels = [ | ||
| scheduling_request.actor_options["label_selector"] or {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is None allowed in actor_options["label_selector"]?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah None is currently allowed there, it defaults to an empty dict {} which results in no label constraints
| fallback_labels = [fallback.get("label_selector", {}) or {}] | ||
| placement_candidates.append( | ||
| (scheduling_request.required_resources, fallback_labels) | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can there be a fallback with label_selector == None?
Also should we skip adding to placement_candidates if fallback_labels is empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah there can be fallback with label_selector == None, it is treated as an empty dictionary which is no label constraints. If a user adds a fallback strategy with no labels, resulting in fallback_labels being empty, I think we still need to add it to placement candidates so that it tries the "match all nodes regardless of labels" behavior.
Co-authored-by: Abrar Sheikh <[email protected]> Signed-off-by: Ryan O'Leary <[email protected]>
Signed-off-by: ryanaoleary <[email protected]> Trim whitepsace and fix invalid test case Signed-off-by: ryanaoleary <[email protected]>
Signed-off-by: ryanaoleary <[email protected]>
…ests for this Signed-off-by: ryanaoleary <[email protected]>
|
@abrarsheikh I believe I've resolved all the outstanding comments. Also in 21bd155, I added some logic where if a single |
abrarsheikh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Left a few nits.
In python/ray/serve/tests/test_deployment_scheduler.py please include tests for non-happy path, eg: when no label selectors can be satisfied, specially interested in the PACK strategy case.
Signed-off-by: ryanaoleary <[email protected]>
Signed-off-by: ryanaoleary <[email protected]>
|
Created a bug fix for an issue I found with |
Signed-off-by: ryanaoleary <[email protected]>
Signed-off-by: ryanaoleary <[email protected]>
Signed-off-by: ryanaoleary <[email protected]>
abrarsheikh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will let @MengjinYan approve, and then I can merge
| return 0 | ||
|
|
||
| cdef extern from * nogil: | ||
| """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is better to add the setLabels() function in cluster_resource_data.h c++ to avoid embedded code here.
Why are these changes needed?
This PR adds the
label_selectoroption to the supported list of Actor options for a Serve deployment. Additionally, we addbundle_label_selectorto specify label selectors for bundles whenplacement_group_bundlesare specified for the deployment. These two options are already supported for Tasks/Actors and placement groups respectively.Example use case:
The expected behaviors of these new fields is as follows:
Pack scheduling enabled
PACK/STRICT_PACK PG strategy:
Standard PG without bundle_label_selector or fallback:
PG node label selector provided:
PG node label selector and fallback:
Same as above but when scheduling tries the following:
SPREAD/STRICT_SPREAD PG strategy:
Spread scheduling enabled
Related issue number
#51564
Checks
git commit -s) in this PR.method in Tune, I've added it in
doc/source/tune/api/under thecorresponding
.rstfile.